cli: add ignition support by enxebre · Pull Request #3120 · coreos/tectonic-installer

enxebre · 2018-03-16T15:23:15Z

Add a IgnitionFile field for node pools
the cli validate ign file and embed the tnc append block
Terraform always receive the generated user-data ign file path as an input

Once we get #3079 will create a follow up PR for etcd to use the generated userdata

coreosbot · 2018-03-16T15:23:16Z

Can one of the admins verify this patch?

enxebre · 2018-03-16T15:24:40Z

@thorfour seems that tnc is ignoring the KubeconfigFetchCmd property

squat

Looks promising. There are a few errors

squat · 2018-03-16T15:26:07Z

config.tf

+  type = "string"
+
+  description = <<EOF
+(optional) Ignition config file path. This is automatically generated by the installer.


Should we change these to (internal) since users should not touch it?

What do you think about supplying the full json here rather than a file path? This way the tectonic CLI does not need to write anything to disk, just add the text to the terraform.tfvars.

Technically make sense, from a user perspective I like the file input

squat · 2018-03-16T15:29:49Z

installer/pkg/config-generator/ignition.go

+}
+
+func (c ConfigGenerator) GenerateIgnConfig(clusterDir string) error {
+


extra newline

squat · 2018-03-16T15:31:47Z

installer/pkg/config-generator/ignition.go

+	return poolToRole
+}
+
+func (c ConfigGenerator) GenerateIgnConfig(clusterDir string) error {


Can you add a comment to this exported func?

squat · 2018-03-16T15:33:37Z

installer/pkg/config-generator/ignition.go

+}
+
+func (c ConfigGenerator) embedAppendBlock(ignCfg ignconfigtypes.Config, role string) *ignconfigtypes.Config {
+


extra newline

squat · 2018-03-16T15:34:33Z

installer/pkg/config-generator/ignition.go

+
+func (c ConfigGenerator) embedAppendBlock(ignCfg ignconfigtypes.Config, role string) *ignconfigtypes.Config {
+
+	appendBlock := &ignconfigtypes.ConfigReference{


why do we generate a pointer to the struct and then de-reference it in the next statement?

squat · 2018-03-16T15:47:32Z

installer/pkg/config/types.go


+type nodePools []nodePool
+
+type nodePool struct {


It does not look like anyone is using this type? https://github.com/enxebre/tectonic-installer/blob/07fe3dab9cc74f8184bb23b821063c8f75eaacda/installer/pkg/config/cluster.go#L36

squat · 2018-03-16T15:48:38Z

installer/pkg/config/validate.go

 	return fmt.Sprintf("node pools cannot be shared, but %q is used by %s", e.name, strings.Join(e.fields, ", "))
 }

+// ErrWrongIgnConfig is returned when a wrong ign config is given.


what do you mean by wrong? Invalid maybe?

squat · 2018-03-16T15:48:56Z

installer/pkg/config/validate.go

+}
+
+func (c *Cluster) validateIgnitionFiles() []error {
+


extra newline

squat · 2018-03-16T15:50:39Z

installer/pkg/config/validate.go

 }

+func validateFileExist(ignitionFile string) error {
+	if _, err := os.Stat(ignitionFile); err != nil {


there is no need for the if here. Just always return err. This way, if the error is nil, then you are returning nil and if the error is not nil then you are returning an error

squat · 2018-03-16T15:50:46Z

installer/pkg/config/validate.go

+}
+
+func validateIgnitionConfig(filePath string) error {
+


extra newline

squat

There are a few errors

enxebre · 2018-03-19T10:37:45Z

@squat addressed the comments PTAL. cc @crawford @alexsomesan

enxebre · 2018-03-19T19:09:53Z

ok to test

enxebre · 2018-03-20T08:26:52Z

test failing due to asg permissions:

* aws_autoscaling_group.workers: AccessDenied: User: arn:aws:sts::846518947292:assumed-role/tf-tectonic-installer/TECTONIC_INSTALLER_awsbas-pr-3120-8630a1 is not authorized to perform: autoscaling:DeleteTags on resource: arn:aws:autoscaling:us-east-2:846518947292:autoScalingGroup:64585609-4a53-4c26-9a37-238442081a0c:autoScalingGroupName/awsbas-pr-3120-8630a1-workers

cc @cpanato

squat · 2018-03-23T08:58:31Z

@enxebre this PR adds too many files. Did you run glide-vc as directed in the README to reduce the number of vendors files?

squat

Overall looks very good. Just a couple small points

squat · 2018-03-23T11:23:01Z

installer/pkg/config-generator/ignition.go

+)
+
+var (
+	ignVersion = ignconfigtypes.IgnitionVersion{


We should probably in-line the this struct for readability

squat · 2018-03-23T11:24:24Z

installer/pkg/config-generator/ignition.go

+		ignFile := p.IgnitionFile
+		ignCfg, err := parseIgnFile(ignFile)
+		if err != nil {
+			return err


We should really wrap error so for improved context when debugging

squat · 2018-03-23T11:25:05Z

installer/pkg/config-generator/ignition.go

+	if err != nil {
+		return nil, err
+	}
+


This error seems to be completely ignored

squat · 2018-03-23T11:26:48Z

installer/pkg/config/cluster.go

 )

+const (
+	IgnitionMaster = "ignition-master.ign"


Could you document these exported values?

spangenberg · 2018-03-23T15:58:39Z

installer/pkg/config/cluster.go

 	openstack.OpenStack `json:",inline" yaml:"openstack,omitempty"`
 	vmware.VMware       `json:",inline" yaml:"vmware,omitempty"`
 	Internal            `json:",inline" yaml:"-"`
+	IgnitionMaster      string `json:"tectonic_ignition_master,omitempty"`


Can you please ignore yaml in this and the below lines.

enxebre · 2018-03-23T17:40:54Z

retest this please

enxebre · 2018-03-27T10:20:22Z

retest this please

The writeFile helpers are from 2b82fbe (installer: Integrate multistep cli with configuration, 2018-02-16, coreos/tectonic-installer#2960) and 461ff5f (cli: add support for user ignition files, 2018-03-23, coreos/tectonic-installer#3120). But ioutil's function has almost the same signature. Remove our local versions and use the stdlib's instead. The \n addition is because ioutil.WriteFile does not append newlines (which workflow.writeFile did via Fprintln). I've used os.Create's 0666 [1] in most cases; callers can set a umask if they want to restrict that. The exception is in generatePrivateKey, where I've set 0600 to avoid leaking a private key even in the presence of a weak umask. [1]: https://golang.org/pkg/os/#Create

The ".ign" suffix is sufficient to identify these as ignition filenames. The old filenames were from 461ff5f (cli: add support for user ignition files, 2018-03-23, coreos/tectonic-installer#3120), which does not motivate the "ingition-" prefix. This gets us closer to the filenames discussed in bf830cc (Documentation/design: add prepare design, 2018-07-11, openshift#50) and b111829 (Documentation/design: add launch design, 2018-07-11, openshift#51).

enxebre added the do-not-merge label Mar 16, 2018

enxebre mentioned this pull request Mar 16, 2018

platform/aws: add user manipulable raw ign config via user-data local files #3090

Closed

squat approved these changes Mar 16, 2018

View reviewed changes

squat suggested changes Mar 16, 2018

View reviewed changes

enxebre force-pushed the ign-cli branch 4 times, most recently from 723229c to bd8da86 Compare March 19, 2018 10:29

enxebre added platform/aws run-smoke-tests and removed do-not-merge labels Mar 19, 2018

enxebre force-pushed the ign-cli branch from a984f12 to bfc6ed8 Compare March 22, 2018 20:40

enxebre force-pushed the ign-cli branch 3 times, most recently from 951ae21 to 53faea3 Compare March 23, 2018 10:54

enxebre requested a review from alexsomesan March 23, 2018 11:19

squat reviewed Mar 23, 2018

View reviewed changes

enxebre force-pushed the ign-cli branch from 53faea3 to afa8559 Compare March 23, 2018 11:53

enxebre requested a review from spangenberg March 23, 2018 14:54

spangenberg suggested changes Mar 23, 2018

View reviewed changes

enxebre force-pushed the ign-cli branch 2 times, most recently from 9645c9c to 20a585e Compare March 23, 2018 16:17

squat previously approved these changes Mar 26, 2018

View reviewed changes

cli: add ignition vendor

6d0846c

enxebre dismissed squat’s stale review via efb528e March 26, 2018 14:35

enxebre force-pushed the ign-cli branch from 20a585e to efb528e Compare March 26, 2018 14:35

squat previously approved these changes Mar 26, 2018

View reviewed changes

enxebre mentioned this pull request Mar 27, 2018

Etcd tnc #3079

Merged

cli: add support for user ignition files

461ff5f

enxebre dismissed squat’s stale review via 461ff5f March 27, 2018 10:02

enxebre force-pushed the ign-cli branch from efb528e to 461ff5f Compare March 27, 2018 10:02

spangenberg approved these changes Mar 27, 2018

View reviewed changes

squat approved these changes Mar 27, 2018

View reviewed changes

squat merged commit 4d3eea6 into coreos:master Mar 27, 2018

wking mentioned this pull request Jul 9, 2018

installer/pkg: Replace writeFile with ioutil.WriteFile openshift/installer#42

Merged

wking mentioned this pull request Jul 23, 2018

installer/pkg: Drop "ignition-" from "ignition-*.ign" filenames openshift/installer#63

Merged

		}

		func (c ConfigGenerator) GenerateIgnConfig(clusterDir string) error {

		}

		func (c ConfigGenerator) embedAppendBlock(ignCfg ignconfigtypes.Config, role string) *ignconfigtypes.Config {


		func (c ConfigGenerator) embedAppendBlock(ignCfg ignconfigtypes.Config, role string) *ignconfigtypes.Config {

		appendBlock := &ignconfigtypes.ConfigReference{

Conversation

enxebre commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreosbot commented Mar 16, 2018

Uh oh!

enxebre commented Mar 16, 2018

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

enxebre commented Mar 19, 2018

Uh oh!

enxebre commented Mar 19, 2018

Uh oh!

enxebre commented Mar 20, 2018

Uh oh!

squat commented Mar 23, 2018

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre commented Mar 23, 2018

Uh oh!

enxebre commented Mar 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

enxebre commented Mar 16, 2018 •

edited

Loading